Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(storage): Implement DiskTokenStorage #19

Merged
merged 3 commits into from
May 20, 2016
Merged

feat(storage): Implement DiskTokenStorage #19

merged 3 commits into from
May 20, 2016

Conversation

dermesser
Copy link
Owner

DiskTokenStorage is a TokenStorage that stores its tokens in a JSON file
on disk. That file can be read in later, and the tokens in it reused.
(The idea for using a cache file is from here:
https://developers.google.com/drive/v3/web/quickstart/go#step_3_set_up_the_sample)

@GitCop
Copy link

GitCop commented Apr 15, 2016

Thanks for contributing. Unfortunately I have to tell you that there were the following issues with your Pull Request

  • Commit: e0249aa
    • Your commit message body contains a line that is longer than 72 characters

Guidelines are available at https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#-git-commit-guidelines


This message was auto-generated by https://gitcop.com

DiskTokenStorage is a TokenStorage that stores its tokens in a JSON file
on disk. That file can be read in later, and the tokens in it reused.
(The idea for a cache file is from here:
https://developers.google.com/drive/v3/web/quickstart/go)
@Byron Byron self-assigned this Apr 23, 2016
@Byron
Copy link
Collaborator

Byron commented Apr 23, 2016

@dermesser Thanks a lot for your contribution ! But believe it or not: I am having serious difficulty to even build this project anymore outside of it's usage in google-apis-rs. Can you give me a hint on how it worked for you ?
The dependency to serde is killing me - even though stable and nightly worked at some point, I really have no clue anymore why it stopped doing so.

};

// best-effort
let _ = dts.load_from_file();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though I like all aspects of the implementation, this is the part I am having a gripe with. Reason for that is that downright ignores all errors, just because there is no way for it to report them. However, to me Rust is the language that makes it easy to not go down that path.

There are two possible solutions coming to my mind:

  • make new() return a Result
  • delay the initial load to the get() and set() methods respectively.

What do you think about this ?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll just make new() return a Result type.

My thoughts were that new() is just "make me a storage; if you can't read it, ignore it, I'll find out when there's no token in it". But you're absolutely right with regards to The Right Way To Do It :-)

@dermesser
Copy link
Owner Author

What kind of error do you get when trying to build? I also had some initial difficulties getting it to build, but I could fix them...

  • I tested my work with a small test crate. When using google-apis-rs from crates.io, but a local version of yup-oauth2 (with my patches), I received weird type errors (Expected XyzStruct but found XyzStruct); fixed by downloading google-apis-rs and making the API crate (I used drive3) use my locally patched version of yup-oauth2 (via yup-oauth2 = { path = "../../../yup-oauth2" })
  • When trying to compile yup-oauth2 (even the direct clone from github), cargo threw an extremely weird error: something like Packageserde_macros v0.6.13does not have these features: 'nightly'. That is Package ndarray v0.4.0 does not have these features: complex rust-lang/cargo#2472 (tl;dr: feature tags leaking across crates o.O) and I solved it by manually installing cargo 0.10.0.

It seems that the latter issue might be what you're encountering. Good luck :-)

@dermesser
Copy link
Owner Author

dermesser commented Apr 25, 2016

hmm, for some reason I get unresolved name form_urlencoded::serialize as error. But also on master D:

@dermesser
Copy link
Owner Author

nvm, that's #24

@@ -14,7 +14,7 @@ build = "src/build.rs"
chrono = ">= 0.2"
log = ">= 0.3"
mime = ">= 0.1"
url = ">= 0.5"
url = "= 0.5"
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this probably ends up as merge conflict (and I'll revert it), but otherwise it doesn't build

@dermesser
Copy link
Owner Author

friendly ping :-)

(just saw that a commit fixing the dependencies made it into this PR. If I should remove it, tell me)

@Byron
Copy link
Collaborator

Byron commented May 12, 2016

Oh, please don't remove it, it is just that I didn't thoroughly look at it yet since the first review. Next week I am on holiday and will thus have time. Do you think that is soon enough?

@dermesser
Copy link
Owner Author

oh sure! I just pinged it to avoid the PR(s) being buried and forgotten. As long as I know that you know about it, I'm fine :)

@Byron
Copy link
Collaborator

Byron commented May 20, 2016

I managed to get it to build on nightly now, using mostly latest versions of dependencies. Stable can't work anymore, apparently due to this issue.

My goal is to bring google-apis-rs back to work in nightly for now, and if necessary do whatever is needed to resolve the aster assertion error first referenced here for good.

@Byron Byron merged commit 95ecb5c into dermesser:master May 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants